fix: Scalar parameter docstring and example arguments unused #479
Conversation
maxburs
commented
Aug 22, 2024
•
maxburs
commented
- Fixed scalar parameter docstring and example arguments not passed along when creating ctor versions
- Improved Playwright config
- Removed our custom timeout config, which was much lower than the defaults
- Use recommended reporters in CI (github + html)
- Upload html report, so we can see why tests failed
| @@ -1695,7 +1695,7 @@ class KustoLanguageService implements LanguageService { | |||
| const paramSymbol: sym.ScalarSymbol = Kusto.Language.Symbols.ScalarTypes.GetSymbol( | |||
| getCslTypeNameFromClrType(param.type) | |||
| ); | |||
| return new sym.ParameterSymbol(param.name, paramSymbol, null); | |||
| return new sym.ParameterSymbol(param.name, paramSymbol, param.docstring ?? null); | |||
There was a problem hiding this comment.
Tested this change, others are a bit more speculative
There was a problem hiding this comment.
I'm not clear on the use case.
Could we add a unit or integration test to clarify it and help prevent regressions?
There was a problem hiding this comment.
Examples and docstring/description are added to tooltips on hover. I'm using them for dashboard system parameters, and and I'd assume other ones are in the system we get from kusto (otherwise, IDK why the C# api would expose it)
Could we add a unit or integration test to clarify it and help prevent regressions?
Unlike a lot of other stuff, this isn't really load bearing. If it breaks it won't block a release. I don't think it should be a priority for testing.
There was a problem hiding this comment.
If a unit test would help I might add that, because it would be easy, but I don't think it would do anything useful.
An integration tests would be pretty expensive to add.
There was a problem hiding this comment.
I'm sorry if I wasn't clear.
From my perspective, testing is an important part of our work as engineers.
It helps ensure that what we build remains reliable and understandable for other developers.
I would suggest adding an integration test here, but if that's too difficult, a unit test would also be fine.
There was a problem hiding this comment.
@maxburs can you add some examples/screenshots of where this shows up and how?
That would help in understanding how/what test we should add.
|
@maxburs I'd like this to be tested. This test will easily find other related regressions. I've opened a task this sprint to complete this task (with an integration test :) ) |
…sers/maburson/parameters
|
@morgilad Tests added |
|
@maxburs the build is failing because of issues with other tests. |
|
@morgilad CI is passing 🎊 Changes since you last took a look:
|
| @@ -41,3 +41,11 @@ jobs: | |||
| - name: integration tests | |||
| working-directory: package | |||
| run: yarn test:integration | |||
|
|
|||
| # https://playwright.dev/docs/ci-intro#html-report | |||
| - uses: actions/upload-artifact@v4 | |||
There was a problem hiding this comment.
Where can I see this report?
